From: Ian Campbell Date: Tue, 24 May 2011 15:18:19 +0000 (+0100) Subject: libxl: don't close file descriptors 4..255 in libxl__exec X-Git-Tag: archive/raspbian/4.8.0-1+rpi1~1^2~10316 X-Git-Url: https://dgit.raspbian.org/%22http:/www.example.com/cgi/%22https:/%22bookmarks://%22/%22http:/www.example.com/cgi/%22https:/%22bookmarks:/%22?a=commitdiff_plain;h=bdf5d459953372a6e0347258ebc15ec58af0bf7d;p=xen.git libxl: don't close file descriptors 4..255 in libxl__exec It prevents callers from deliberately passing file descriptors to the child, hides any callers who erroneously do so and doesn't deal with all file descriptors in any case. Rather than remove it all together replace it with some debug code which checks for open file handles which do not have either O_CLOEXEC or FD_CLOEXEC set. To enable the debug set _LIBXL_DEBUG_EXEC_FDS=1 to print any open and non-CLOEXEC non-stdio FDs just before libxl__exec actualy calls exec. Set _LIBXL_DEBUG_EXEC_FDS=2 to abort if any of these exist. On the basis of this debugging fix some leaked filehandles: * The read end of the pipe used to wake the parent from the intermediate process during libxl__spawn_spawn was leaked into the intermediate process. * The file descriptor representing the xl lock was not marked O_CLOEXEC (the lock itself is already specified to not be inherited across a fork). * The file descriptors passed to libxl__exec to be dup'd as std{in,out,err} were leaked at their original number. They are harmless (an attacker can just as easily use fd 0..2) but close anyway since it removes a case which a person evaluating open fd's needs to consider. * libxl_run_bootloader was leaking the xenconsole pty master into the bootloader child process. * If the bootloader fails to get as far as opening its end of the FIFO then we can also hang, check that the process has not exited as part of that loop. (we actually block opening the FIFO too so this is only a partial fix for the case where the bootlader has crashed quickly). With these fixes I have tested that device models, bootloaders (pygrub) and vncviewers which are spawned via libxl__exec with no unexpected file descriptors open, at least in my configuration. Signed-off-by: Ian Campbell Acked-by: Ian Jackson Committed-by: Ian Jackson --- diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c index 3eb0343bf9..0d0b8d7652 100644 --- a/tools/libxl/libxl_bootloader.c +++ b/tools/libxl/libxl_bootloader.c @@ -115,6 +115,7 @@ static int open_xenconsoled_pty(int *master, int *slave, char *slave_path, size_ #endif fcntl(*master, F_SETFL, O_NDELAY); + fcntl(*master, F_SETFD, FD_CLOEXEC); return 0; } @@ -393,6 +394,9 @@ int libxl_run_bootloader(libxl_ctx *ctx, } while (1) { + if (waitpid(pid, &blrc, WNOHANG) == pid) + goto out_close; + fifo_fd = open(fifo, O_RDONLY); if (fifo_fd > -1) break; diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c index ce6e93f4f0..27fd6cd600 100644 --- a/tools/libxl/libxl_exec.c +++ b/tools/libxl/libxl_exec.c @@ -36,20 +36,72 @@ static int call_waitpid(pid_t (*waitpid_cb)(pid_t, int *, int), pid_t pid, int * return (waitpid_cb) ? waitpid_cb(pid, status, options) : waitpid(pid, status, options); } +static void check_open_fds(const char *what) +{ + const char *env_debug; + int debug; + int i, flags, badness = 0; + char path[PATH_MAX]; + char link[PATH_MAX+1]; + + env_debug = getenv("_LIBXL_DEBUG_EXEC_FDS"); + if (!env_debug) return; + + debug = strtol(env_debug, (char **) NULL, 10);atoi(env_debug); + if (debug <= 0) return; + + for (i = 4; i < 256; i++) { +#ifdef __linux__ + size_t len; +#endif + flags = fcntl(i, F_GETFD); + if ( flags == -1 ) { + if ( errno != EBADF ) + fprintf(stderr, "libxl: execing %s: fd %d flags returned %s (%d)\n", + what, i, strerror(errno), errno); + continue; + } + + if ( flags & FD_CLOEXEC ) + continue; + + badness++; + +#ifdef __linux__ + snprintf(path, PATH_MAX, "/proc/%d/fd/%d", getpid(), i); + len = readlink(path, link, PATH_MAX); + if (len > 0) { + link[len] = '\0'; + fprintf(stderr, "libxl: execing %s: fd %d is open to %s with flags %#x\n", + what, i, link, flags); + } else +#endif + fprintf(stderr, "libxl: execing %s: fd %d is open with flags %#x\n", + what, i, flags); + } + if (debug < 2) return; + if (badness) abort(); +} + void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0, char **args) /* call this in the child */ { - int i; - if (stdinfd != -1) dup2(stdinfd, STDIN_FILENO); if (stdoutfd != -1) dup2(stdoutfd, STDOUT_FILENO); if (stderrfd != -1) dup2(stderrfd, STDERR_FILENO); - for (i = 4; i < 256; i++) - close(i); + + if (stdinfd != -1) + close(stdinfd); + if (stdoutfd != -1 && stdoutfd != stdinfd) + close(stdoutfd); + if (stderrfd != -1 && stderrfd != stdinfd && stderrfd != stdoutfd) + close(stderrfd); + + check_open_fds(arg0); signal(SIGPIPE, SIG_DFL); /* in case our caller set it to IGN. subprocesses are entitled @@ -148,6 +200,7 @@ int libxl__spawn_spawn(libxl__gc *gc, } /* we are now the intermediate process */ + if (for_spawn) close(pipes[0]); child = fork(); if (child == -1) diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 5415bc51a3..159677a110 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -199,7 +199,7 @@ static int acquire_lock(void) fl.l_whence = SEEK_SET; fl.l_start = 0; fl.l_len = 0; - fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR); + fd_lock = open(lockfile, O_WRONLY|O_CREAT|O_CLOEXEC, S_IWUSR); if (fd_lock < 0) { fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile, errno); return ERROR_FAIL;